[ECO-5253] Use a single channel for all features#247
[ECO-5253] Use a single channel for all features#247lawrence-forooghian merged 9 commits intomainfrom
Conversation
WalkthroughThis change implements a major architectural refactor to unify all chat room features (messages, presence, reactions, occupancy, typing) onto a single Ably realtime channel per room. It removes the previous multi-channel and feature-contributor abstractions, eliminates feature-specific attach/detach error codes, and introduces a room-level discontinuity event API. The Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant Rooms
participant Room
participant LifecycleManager
participant RealtimeChannel
App->>Rooms: get(roomID)
Rooms->>Room: createRoom(roomID, options)
Room->>RealtimeChannel: create (single channel)
Room->>LifecycleManager: manage(channel)
App->>Room: presence/messages/reactions/occupancy/typing
Room->>RealtimeChannel: use single channel for all features
App->>Room: onDiscontinuity()
Room->>LifecycleManager: subscribe to discontinuity events
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
lawrence-forooghian
force-pushed
the
242-single-channel
branch
from
b477683 to
44fc705
Compare
lawrence-forooghian
force-pushed
the
242-single-channel
branch
from
44fc705 to
6034f8e
Compare
lawrence-forooghian
force-pushed
the
242-single-channel
branch
from
86fae73 to
3e04a84
Compare
lawrence-forooghian
force-pushed
the
242-single-channel
branch
from
3e04a84 to
6c4b326
Compare
lawrence-forooghian
force-pushed
the
242-single-channel
branch
from
6c4b326 to
642bbbc
Compare
lawrence-forooghian
force-pushed
the
242-single-channel
branch
from
642bbbc to
0b0a894
Compare
lawrence-forooghian
force-pushed
the
242-single-channel
branch
from
0b0a894 to
da2ba9e
Compare
lawrence-forooghian
force-pushed
the
242-single-channel
branch
from
da2ba9e to
2adf4cb
Compare
lawrence-forooghian
force-pushed
the
242-single-channel
branch
from
2adf4cb to
6639b34
Compare
lawrence-forooghian
changed the base branch from
main
to
spec-coverage-improvements
lawrence-forooghian
force-pushed
the
242-single-channel
branch
from
6639b34 to
a56c0a2
Compare
lawrence-forooghian
force-pushed
the
242-single-channel
branch
from
a56c0a2 to
6150d33
Compare
lawrence-forooghian
changed the base branch from
spec-coverage-improvements
to
ECO-5241
lawrence-forooghian
force-pushed
the
242-single-channel
branch
from
6150d33 to
82e1bee
Compare
umair-ably
force-pushed
the
ECO-5241
branch
3 times, most recently
from
3178d4d to
dbb9534
Compare
umair-ably
force-pushed
the
ECO-5241
branch
2 times, most recently
from
f00c681 to
e3afbd8
Compare
maratal
left a comment
maratal
left a comment
There was a problem hiding this comment.
Looks good, I've briefly checked tests changes since they are just corrections for code changes. Left one minor note as well.
lawrence-forooghian
force-pushed
the
242-single-channel
branch
from
da584f6 to
274e60f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
Sources/AblyChat/RoomLifecycleManager.swift (1)
566-568:⚠️ Potential issue
try! clock.sleepwill crash onTaskcancellationThe TODO from the previous review remains. Using
try!around an async sleep traps the process if the enclosing task is cancelled (e.g. app backgrounded). Replace with a safe variant:-// swiftlint:disable:next force_try -try! await clock.sleep(timeInterval: waitDuration) +do { + try await clock.sleep(timeInterval: waitDuration) +} catch is CancellationError { + logger.log(message: "Release task cancelled during back‑off sleep", level: .debug) + return // propagate or handle cancellation gracefully +}
🧹 Nitpick comments (3)
Tests/AblyChatTests/DefaultRoomTests.swift (1)
10-22: Good test for channel naming conventionThis test properly verifies that the internal channel naming follows the expected format with the single channel model. However, you should update the
@specreference in the comment to match the specific specification item.Sources/AblyChat/Room.swift (1)
205-209: Property comment warns not to usechannel, but protocol makes it publicThe inline comment discourages internal use of
channel, yet the protocol exposes it as an official API. This inconsistency will confuse maintainers and end‑users. Either:
- Rename the internal property (e.g.
_unsafeChannel) and keepchannelfor public use, or- Remove the comment and treat
channelas a first‑class, supported API.Sources/AblyChat/RoomLifecycleManager.swift (1)
166-174: De‑init performs async work after state capture – possible listener leakCapturing
channelStateEventListenerbefore hopping to aTaskis correct, but if the actor is de‑initialised while another operation still references the channel, the listener removal might never execute (task cancelled). Consider:deinit { let listener = channelStateEventListener! let chan = channel Task { @MainActor in chan.off(listener) } }or convert to
withUnsafeContinuationand synchronously remove the listener to guarantee cleanup.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (44)
Example/AblyChatExample/ContentView.swift(3 hunks)Example/AblyChatExample/Mocks/MockClients.swift(4 hunks)Package.swift(3 hunks)README.md(1 hunks)Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift(4 hunks)Sources/AblyChat/ChatAPI.swift(5 hunks)Sources/AblyChat/ChatClient.swift(1 hunks)Sources/AblyChat/DefaultMessages.swift(10 hunks)Sources/AblyChat/DefaultOccupancy.swift(3 hunks)Sources/AblyChat/DefaultPresence.swift(14 hunks)Sources/AblyChat/DefaultRoomLifecycleContributor.swift(0 hunks)Sources/AblyChat/DefaultRoomReactions.swift(4 hunks)Sources/AblyChat/DefaultTyping.swift(1 hunks)Sources/AblyChat/DiscontinuityEvent.swift(1 hunks)Sources/AblyChat/EmitsDiscontinuities.swift(0 hunks)Sources/AblyChat/Errors.swift(6 hunks)Sources/AblyChat/Messages.swift(1 hunks)Sources/AblyChat/Occupancy.swift(1 hunks)Sources/AblyChat/Presence.swift(3 hunks)Sources/AblyChat/Room.swift(6 hunks)Sources/AblyChat/RoomFeature.swift(1 hunks)Sources/AblyChat/RoomLifecycleManager.swift(12 hunks)Sources/AblyChat/RoomOptions.swift(2 hunks)Sources/AblyChat/RoomReactionDTO.swift(3 hunks)Sources/AblyChat/RoomReactions.swift(1 hunks)Sources/AblyChat/RoomStatus.swift(3 hunks)Sources/AblyChat/Rooms.swift(3 hunks)Sources/AblyChat/Subscription.swift(1 hunks)Sources/AblyChat/Typing.swift(3 hunks)Tests/AblyChatTests/DefaultMessagesTests.swift(5 hunks)Tests/AblyChatTests/DefaultPresenceTests.swift(1 hunks)Tests/AblyChatTests/DefaultRoomOccupancyTests.swift(2 hunks)Tests/AblyChatTests/DefaultRoomReactionsTests.swift(2 hunks)Tests/AblyChatTests/DefaultRoomTests.swift(8 hunks)Tests/AblyChatTests/DefaultRoomsTests.swift(13 hunks)Tests/AblyChatTests/IntegrationTests.swift(5 hunks)Tests/AblyChatTests/Mocks/MockFeatureChannel.swift(0 hunks)Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift(2 hunks)Tests/AblyChatTests/Mocks/MockRoom.swift(1 hunks)Tests/AblyChatTests/Mocks/MockRoomLifecycleContributor.swift(0 hunks)Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift(2 hunks)Tests/AblyChatTests/Mocks/MockRoomLifecycleManagerFactory.swift(1 hunks)Tests/AblyChatTests/RoomOptionsTests.swift(1 hunks)Tests/AblyChatTests/RoomReactionDTOTests.swift(2 hunks)
💤 Files with no reviewable changes (4)
- Sources/AblyChat/DefaultRoomLifecycleContributor.swift
- Tests/AblyChatTests/Mocks/MockRoomLifecycleContributor.swift
- Tests/AblyChatTests/Mocks/MockFeatureChannel.swift
- Sources/AblyChat/EmitsDiscontinuities.swift
✅ Files skipped from review due to trivial changes (3)
- README.md
- Sources/AblyChat/DiscontinuityEvent.swift
- Sources/AblyChat/Rooms.swift
🚧 Files skipped from review as they are similar to previous changes (30)
- Tests/AblyChatTests/RoomReactionDTOTests.swift
- Sources/AblyChat/RoomReactions.swift
- Tests/AblyChatTests/DefaultMessagesTests.swift
- Sources/AblyChat/ChatAPI.swift
- Tests/AblyChatTests/DefaultPresenceTests.swift
- Sources/AblyChat/Subscription.swift
- Sources/AblyChat/ChatClient.swift
- Example/AblyChatExample/ContentView.swift
- Package.swift
- Sources/AblyChat/Presence.swift
- Sources/AblyChat/RoomReactionDTO.swift
- Sources/AblyChat/Messages.swift
- Tests/AblyChatTests/Mocks/MockRoom.swift
- Tests/AblyChatTests/RoomOptionsTests.swift
- Tests/AblyChatTests/DefaultRoomsTests.swift
- Tests/AblyChatTests/DefaultRoomReactionsTests.swift
- Tests/AblyChatTests/Mocks/MockRoomLifecycleManagerFactory.swift
- Tests/AblyChatTests/DefaultRoomOccupancyTests.swift
- Sources/AblyChat/Occupancy.swift
- Sources/AblyChat/RoomStatus.swift
- Sources/AblyChat/RoomOptions.swift
- Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift
- Sources/AblyChat/DefaultRoomReactions.swift
- Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift
- Sources/AblyChat/DefaultOccupancy.swift
- Tests/AblyChatTests/IntegrationTests.swift
- Sources/AblyChat/DefaultPresence.swift
- Sources/AblyChat/DefaultMessages.swift
- Example/AblyChatExample/Mocks/MockClients.swift
- Sources/AblyChat/Errors.swift
🧰 Additional context used
🧬 Code Graph Analysis (2)
Tests/AblyChatTests/DefaultRoomTests.swift (3)
Sources/AblyChat/Room.swift (4)
onStatusChange(132-134)onStatusChange(324-326)onDiscontinuity(136-138)onDiscontinuity(334-336)Tests/AblyChatTests/Mocks/MockRoom.swift (2)
onStatusChange(43-45)onDiscontinuity(71-73)Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (3)
emitStatusChange(59-61)onDiscontinuity(67-69)emitDiscontinuity(71-73)
Sources/AblyChat/Room.swift (9)
Sources/AblyChat/RoomLifecycleManager.swift (2)
onDiscontinuity(221-223)createManager(39-48)Tests/AblyChatTests/DefaultRoomTests.swift (3)
onDiscontinuity(284-306)onStatusChange(257-279)release(211-233)Tests/AblyChatTests/Mocks/MockRoom.swift (3)
onDiscontinuity(71-73)onStatusChange(43-45)release(55-62)Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (1)
onDiscontinuity(67-69)Sources/AblyChat/Rooms.swift (4)
createRoom(307-312)get(60-63)get(167-274)release(328-366)Tests/AblyChatTests/Mocks/MockRoomLifecycleManagerFactory.swift (1)
createManager(11-14)Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift (1)
release(171-173)Sources/AblyChat/DefaultRoomReactions.swift (2)
subscribe(14-16)subscribe(53-105)Sources/AblyChat/DefaultMessages.swift (4)
subscribe(18-20)subscribe(62-157)get(22-24)get(160-166)
🔇 Additional comments (12)
Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift (3)
93-110: Safely ensuring callbacks execute on the main threadThis helper function effectively ensures that callbacks from Ably are executed on the main thread, which is a requirement of the SDK. The implementation properly uses
preconditionIsolatedto validate thread requirements andassumeIsolatedto safely execute the callback.
112-119: Careful use of @unchecked SendableThe
UnsafeSendingBoxclass appropriately uses@unchecked Sendableto bypass Swift's concurrency checks for non-Sendable types. The warning comment clearly documents the associated risks and assumptions about Ably-cocoa's behavior.
249-251: Consistent application of main thread safetyThe implementation consistently applies the
toAblyCocoaCallbackwrapper to all event callbacks, ensuring proper thread isolation throughout the codebase.Also applies to: 253-255, 365-367, 369-371, 409-411
Sources/AblyChat/RoomFeature.swift (1)
3-10: Well-structured simplification for single channel architectureThe simplification of the
RoomFeatureenum aligns perfectly with the PR objectives of using a single channel for all features. Removing channel naming utilities and theFeatureChannelabstraction helps streamline the codebase.Sources/AblyChat/Typing.swift (3)
10-10: Streamlined protocol definitionRemoving the
EmitsDiscontinuitiesconformance from theTypingprotocol aligns with the architectural simplification to handle discontinuities at the room level rather than per feature.
33-44: Clear API semantics with the keystroke methodThe renaming of
start()tokeystroke()along with the updated documentation provides better semantics around typing indicators. The documentation clearly explains the new heartbeat throttle approach.
70-88: Enhanced typing event modelThe addition of the
Changestruct toTypingEventprovides more detailed information about typing state changes, making it easier for clients to understand which user triggered a typing event and what type of event occurred.Tests/AblyChatTests/DefaultRoomTests.swift (4)
43-60: Appropriate test update for single channel architectureThe test has been correctly renamed and updated to verify that only a single channel is created and properly configured with merged options from multiple features.
62-118: Comprehensive parameter testing for feature flagsThese parameterized tests thoroughly verify how feature flags affect channel configuration, ensuring that enabling/disabling presence and occupancy events correctly sets channel parameters and modes.
123-134: Simplified lifecycle management testThis test now correctly verifies that the lifecycle manager is created with the exact channel instance, reflecting the removal of the contributor pattern in favor of direct channel management.
282-309: Important test for discontinuity eventsThis new test properly verifies that discontinuity events emitted by the lifecycle manager propagate correctly through the room's API, ensuring that clients can be notified of connectivity issues.
Sources/AblyChat/Room.swift (1)
277-297:Details
❌ Incorrect review comment
Room options → channel‐options mapping looks inverted
if !roomOptions.presence.enableEvents { … modes = [.publish, .subscribe, .presence] }If events are disabled you still include the
.presencemode. According to CHA‑PR9c2 the intent is “do not subscribe to presence events” – i.e. remove.presencewhenenableEvents == false. Please double‑check the spec wording; subscribing to presence when the client explicitly opted‑out leaks PII and wastes bandwidth.-if !roomOptions.presence.enableEvents { - channelOptions.modes = [.publish, .subscribe, .presence] -} +if !roomOptions.presence.enableEvents { + channelOptions.modes = [.publish, .subscribe] // drop .presence +}
🌐 Web query:
Ably Swift ARTRealtimeChannelOptions modes property documentation💡 Result:
Ably Swift
ARTRealtimeChannelOptions.modesProperty DocumentationPurpose and Overview
The
modesproperty inARTRealtimeChannelOptions(and related classes) allows Ably clients to specify channel mode flags. These flags control the capabilities the client wishes to use on a specific channel, such as publishing, subscribing, or participating in presence. This is a way for a client to limit its capabilities on a channel, even if its credentials technically permit more actions[3].Channel mode flags offer the ability for clients to use different capabilities for different channels, however, as they are flags and not permissions they cannot be enforced by an authentication server[3].
Available Mode Flags
Flag Description PUBLISHCan publish messages to the channel SUBSCRIBECan subscribe to receive messages on the channel PRESENCECan register presence on the channel PRESENCE_SUBSCRIBECan subscribe to receive presence events Usage in Swift
You can set the
modesproperty when creating or modifying channel options in Swift. This is useful for restricting actions such as publishing or subscribing for a given channel, regardless of what the credentials allow.Example Usage
let channelOptions = ARTRealtimeChannelOptions() channelOptions.modes = [.publish, .subscribe] // Only allow publish and subscribe let channel = client.channels.get("my-channel", options: channelOptions)Note: In Swift, the modes are typically set using an OptionSet or array of enums/strings representing the desired capabilities. Check the Ably Swift SDK docs for the exact syntax for the version you are using.
Example: Presence Without Subscribing
You can allow a client to be present on a channel (e.g., show up in presence set) without receiving presence events:
channelOptions.modes = [.presence] // Only participate in presence, not receive presence eventsBehavior
- The
modesflags limit the client's channel-side actions. For example, withoutPUBLISH, the client cannot send messages, and withoutSUBSCRIBE, it cannot receive them on that channel.- These flags cannot enforce permissions server-side; they only limit what the client attempts to do[3].
Setting modes at runtime
You can adjust the
modesproperty viasetOptions():channel.setOptions(channelOptions)This will trigger a channel re-attachment if the channel is already attached, or will take effect upon initial attachment[3].
Summary Table
Mode Flag Allows client to... PUBLISHPublish messages to the channel SUBSCRIBESubscribe to messages from the channel PRESENCERegister itself in presence set PRESENCE_SUBSCRIBEReceive presence events References
- Ably channel options documentation for modes[3]
- Ably Swift API and usage patterns[6]
Citations:
- 1: https://ably.com/docs/sdk/cocoa/v1.2/Classes/ARTChannelOptions.html
- 2: https://github.com/ably/ably-cocoa
- 3: https://ably.com/docs/channels/options
- 4: https://github.com/ably/docs/blob/main/content/api/realtime-sdk/presence.textile
- 5: https://cocoapods.org/pods/AblyRealtime
- 6: https://github.com/ably/docs/blob/main/content/channels/options/rewind.textile
- 7: https://cocoapods.org/pods/Ably/changelog
- 8: https://ably.com/docs/sdk/cocoa/v1.2/Classes/ARTRealtimeChannel.html
- 9: https://pub.dev/packages/ably_flutter/changelog
- 10: https://www.producthunt.com/products/ably-realtime/alternatives
🏁 Script executed:
#!/bin/bash # Search for ARTRealtimeChannelOptions definition and its modes type in the repository rg -R "class ARTRealtimeChannelOptions" -A5 || rg -R "struct ARTRealtimeChannelOptions" -A5Length of output: 902
Incorrect assumption on presence flag mapping
The review comment conflates
.presence(registering presence) with.presence_subscribe(receiving presence events). In our code we explicitly set:channelOptions.modes = [.publish, .subscribe, .presence]– we include only the registration flag and omit the subscribe flag, so the client won’t receive presence events when
enableEvents == false. No change is needed here.Likely an incorrect or invalid review comment.
lawrence-forooghian
force-pushed
the
ECO-5241
branch
2 times, most recently
from
f2f8756 to
80af097
Compare
umair-ably
force-pushed
the
ECO-5241
branch
2 times, most recently
from
a87f929 to
3b85aaa
Compare
lawrence-forooghian
force-pushed
the
242-single-channel
branch
from
274e60f to
364dc6f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
Sources/AblyChat/Room.swift (1)
123-129:⚠️ Potential issueNonisolated
channelexposes a non‑thread‑safe Ably objectReturning the raw
RealtimeChannelProtocolfrom anonisolatedproperty defeats Swift's actor isolation and lets concurrent tasks touch the underlyingARTRealtimeChannelwithout synchronization, leading to data races. Please choose one of the following:• Wrap all channel operations in a façade that hops to
@MainActor(or the actor instance) before forwarding calls.
• AnnotateRealtimeChannelProtocol(or thechannelproperty) with@unchecked Sendableand document that callers must confine usage to the main actor.Sources/AblyChat/RoomLifecycleManager.swift (1)
566-568: Address Task Cancellation in Sleep OperationThe code uses
try! await clock.sleep(timeInterval: waitDuration)without handling the case when theTaskis cancelled. As verified, issue #29 remains open and highlights the need to decide how the SDK should interact with Swift's Task cancellation—especially what to do whenTask.sleep(…)fails with a cancellation error.
- Current Concern: Using
try!will trap if the sleep operation is cancelled.- Actionable Suggestion: Update this block to safely handle cancellation (e.g., using proper error handling or a cancellation check) or, at a minimum, document the provisional limitations until a design decision is made.
🧹 Nitpick comments (1)
Sources/AblyChat/Room.swift (1)
205-213: Consider combining testsOnly propertiesThe
testsOnly_internalChannelproperty exists alongside a comment instructing not to use thechannelproperty internally. Consider combining these concerns by:
- Making
internalChannela private property for improved encapsulation- Exposing it only when needed for testing
- Adding a compile-time warning (like
@available(*, deprecated, message: "For tests only")) to the test property
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (48)
.github/workflows/check.yaml(2 hunks)Example/AblyChatExample/ContentView.swift(2 hunks)Example/AblyChatExample/Mocks/MockClients.swift(2 hunks)Package.swift(0 hunks)README.md(1 hunks)Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift(4 hunks)Sources/AblyChat/ChatAPI.swift(5 hunks)Sources/AblyChat/ChatClient.swift(1 hunks)Sources/AblyChat/DefaultMessages.swift(10 hunks)Sources/AblyChat/DefaultOccupancy.swift(3 hunks)Sources/AblyChat/DefaultPresence.swift(14 hunks)Sources/AblyChat/DefaultRoomLifecycleContributor.swift(0 hunks)Sources/AblyChat/DefaultRoomReactions.swift(4 hunks)Sources/AblyChat/DefaultTyping.swift(8 hunks)Sources/AblyChat/DiscontinuityEvent.swift(1 hunks)Sources/AblyChat/EmitsDiscontinuities.swift(0 hunks)Sources/AblyChat/Errors.swift(6 hunks)Sources/AblyChat/Messages.swift(1 hunks)Sources/AblyChat/Occupancy.swift(1 hunks)Sources/AblyChat/Presence.swift(3 hunks)Sources/AblyChat/Room.swift(6 hunks)Sources/AblyChat/RoomFeature.swift(1 hunks)Sources/AblyChat/RoomLifecycleManager.swift(12 hunks)Sources/AblyChat/RoomOptions.swift(2 hunks)Sources/AblyChat/RoomReactionDTO.swift(3 hunks)Sources/AblyChat/RoomReactions.swift(1 hunks)Sources/AblyChat/RoomStatus.swift(3 hunks)Sources/AblyChat/Rooms.swift(3 hunks)Sources/AblyChat/Subscription.swift(1 hunks)Sources/AblyChat/Typing.swift(1 hunks)Tests/AblyChatTests/ChatAPITests.swift(5 hunks)Tests/AblyChatTests/DefaultMessagesTests.swift(5 hunks)Tests/AblyChatTests/DefaultPresenceTests.swift(1 hunks)Tests/AblyChatTests/DefaultRoomOccupancyTests.swift(2 hunks)Tests/AblyChatTests/DefaultRoomReactionsTests.swift(2 hunks)Tests/AblyChatTests/DefaultRoomTests.swift(8 hunks)Tests/AblyChatTests/DefaultRoomsTests.swift(13 hunks)Tests/AblyChatTests/Helpers/RoomLifecycle+Error.swift(0 hunks)Tests/AblyChatTests/IntegrationTests.swift(3 hunks)Tests/AblyChatTests/Mocks/MockFeatureChannel.swift(0 hunks)Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift(3 hunks)Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift(2 hunks)Tests/AblyChatTests/Mocks/MockRoom.swift(1 hunks)Tests/AblyChatTests/Mocks/MockRoomLifecycleContributor.swift(0 hunks)Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift(2 hunks)Tests/AblyChatTests/Mocks/MockRoomLifecycleManagerFactory.swift(1 hunks)Tests/AblyChatTests/RoomOptionsTests.swift(1 hunks)Tests/AblyChatTests/RoomReactionDTOTests.swift(2 hunks)
💤 Files with no reviewable changes (6)
- Package.swift
- Tests/AblyChatTests/Helpers/RoomLifecycle+Error.swift
- Sources/AblyChat/EmitsDiscontinuities.swift
- Tests/AblyChatTests/Mocks/MockRoomLifecycleContributor.swift
- Tests/AblyChatTests/Mocks/MockFeatureChannel.swift
- Sources/AblyChat/DefaultRoomLifecycleContributor.swift
✅ Files skipped from review due to trivial changes (2)
- Tests/AblyChatTests/IntegrationTests.swift
- README.md
🚧 Files skipped from review as they are similar to previous changes (37)
- Tests/AblyChatTests/DefaultPresenceTests.swift
- Example/AblyChatExample/ContentView.swift
- Sources/AblyChat/ChatAPI.swift
- Tests/AblyChatTests/ChatAPITests.swift
- Tests/AblyChatTests/RoomReactionDTOTests.swift
- Sources/AblyChat/Messages.swift
- Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift
- Sources/AblyChat/RoomReactions.swift
- Sources/AblyChat/Typing.swift
- Sources/AblyChat/ChatClient.swift
- Sources/AblyChat/DiscontinuityEvent.swift
- Tests/AblyChatTests/DefaultMessagesTests.swift
- Tests/AblyChatTests/DefaultRoomReactionsTests.swift
- Tests/AblyChatTests/RoomOptionsTests.swift
- Sources/AblyChat/Occupancy.swift
- Tests/AblyChatTests/DefaultRoomOccupancyTests.swift
- Sources/AblyChat/Subscription.swift
- Sources/AblyChat/RoomFeature.swift
- Tests/AblyChatTests/Mocks/MockRoom.swift
- .github/workflows/check.yaml
- Sources/AblyChat/RoomStatus.swift
- Tests/AblyChatTests/Mocks/MockRoomLifecycleManagerFactory.swift
- Sources/AblyChat/Presence.swift
- Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift
- Sources/AblyChat/Rooms.swift
- Sources/AblyChat/RoomReactionDTO.swift
- Sources/AblyChat/DefaultMessages.swift
- Sources/AblyChat/DefaultTyping.swift
- Sources/AblyChat/DefaultPresence.swift
- Sources/AblyChat/DefaultOccupancy.swift
- Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift
- Tests/AblyChatTests/DefaultRoomsTests.swift
- Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift
- Sources/AblyChat/Errors.swift
- Sources/AblyChat/DefaultRoomReactions.swift
- Example/AblyChatExample/Mocks/MockClients.swift
- Sources/AblyChat/RoomOptions.swift
🧰 Additional context used
🧬 Code Graph Analysis (1)
Tests/AblyChatTests/DefaultRoomTests.swift (4)
Sources/AblyChat/Room.swift (4)
onStatusChange(132-134)onStatusChange(325-327)onDiscontinuity(136-138)onDiscontinuity(335-337)Tests/AblyChatTests/Mocks/MockRoom.swift (2)
onStatusChange(43-45)onDiscontinuity(71-73)Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (3)
emitStatusChange(59-61)onDiscontinuity(67-69)emitDiscontinuity(71-73)Sources/AblyChat/RoomLifecycleManager.swift (2)
onDiscontinuity(221-223)emitDiscontinuity(225-227)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Example app, macOS (Xcode 16.3)
- GitHub Check: Generate code coverage (Xcode 16.3)
- GitHub Check: Xcode, tvOS (Xcode 16.3)
- GitHub Check: Generate code coverage (Xcode 16.2)
- GitHub Check: Xcode, iOS (Xcode 16.3)
- GitHub Check: Xcode, macOS (Xcode 16.3)
- GitHub Check: Xcode, tvOS (Xcode 16.2)
- GitHub Check: Xcode, iOS (Xcode 16.2)
- GitHub Check: Xcode, macOS (Xcode 16.2)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16.2) - GitHub Check: lint
🔇 Additional comments (19)
Tests/AblyChatTests/DefaultRoomTests.swift (7)
9-22: Well-designed test for the single channel implementationThe new test verifies that the room uses the expected consolidated channel name
"basketball::$chat"rather than separate feature-specific channels. This aligns with the PR objective to use a single channel for all features.
41-60: Test updated to verify single channel approachThis test has been properly renamed from
fetchesEachChannelOnce()tofetchesChannelOnce()and updated to verify the consolidated channel approach. The assertions now check that only one channel is fetched with the appropriate merged options.
62-118: Great use of parameterized tests for feature configurationsThese parameterized tests thoroughly verify that enabling/disabling occupancy and presence events correctly configures the channel parameters and modes. This is a robust way to test configuration options and ensures the single channel is properly configured based on room options.
122-134: Test simplified to match new architectureThe test
passesChannelToLifecycleManager()properly replaces the previous contributor-based test, verifying that the lifecycle manager receives the exact channel instance. This simplification aligns with the removal of the multi-contributor architecture.
246-246: RoomStatus constructors updated for clarityThe explicit
error: nilparameter makes the code more readable and maintainable by clearly indicating that no error is present.
272-273: RoomStatus constructor parameters updated for claritySimilar to the earlier instance, adding explicit
error: nilparameters improves code clarity.
281-310: Well-implemented test for discontinuity eventsThis new test properly verifies that discontinuity events emitted by the lifecycle manager correctly propagate through the room's
onDiscontinuity()subscription. The comment about async sequences vs explicit unsubscribe APIs is also helpful for understanding the implementation.Sources/AblyChat/Room.swift (5)
108-121: Good addition of discontinuity event subscription APIThe new discontinuity event API allows clients to be notified of connectivity issues. This is an important feature for real-time applications and aligns with the PR objectives.
136-138: Well-designed default implementation for discontinuity eventsThis default implementation follows the same pattern as the existing
onStatusChange()default implementation, providing a convenient way to subscribe with unbounded buffering.
176-186: Factory simplification removes unnecessary generic typeThe simplified return type makes the API more straightforward and aligns with the architectural changes in the PR.
278-298: Well-encapsulated channel creation logicThis static method properly encapsulates the channel configuration logic in one place, making it easier to understand and maintain. The code includes thorough comments referencing specification requirements, and correctly implements the channel options based on room settings.
333-337: Good implementation of discontinuity event handlingThe implementation correctly delegates to the lifecycle manager, following the same pattern as the existing status change subscription.
Sources/AblyChat/RoomLifecycleManager.swift (7)
22-24: Good addition of discontinuity event API to lifecycle managerAdding the
onDiscontinuitymethod to the protocol provides a clean way for rooms to subscribe to discontinuity events from the lifecycle manager.
51-78: Clean RoomStatus initialization from channel stateThis extension provides a clean mapping from Ably channel states to room states, with appropriate error handling for suspended and failed states. The use of
fatalErrorfor unexpected states is appropriate since these would represent implementation bugs.
162-164: Improved channel state event handlingThe simplified approach of using a direct callback instead of
AsyncSequencefor channel state events is more straightforward and avoids potential race conditions in a multi-threaded environment. This helps resolve issue #239 as mentioned in the PR objectives.
195-218: Well-implemented discontinuity detection logicThe method properly identifies discontinuities by checking if the event is not resumed, the room has attached once before, and it's not explicitly detached. The comment explaining the reliance on main thread dispatch is very helpful for understanding the implementation requirements.
221-227: Clean implementation of discontinuity event subscriptionThese methods follow the same pattern as the existing status change subscription implementation, making the code consistent and easier to understand.
386-427: Simplified attach operation implementationThe attach operation has been greatly simplified by removing contributor-specific logic while maintaining the correct behavior. The operation now directly works with the single channel and properly handles errors and status changes.
580-613: Well-updated presence operation checkThe method has been properly updated to use the simplified room status approach while maintaining the correct behavior for waiting for the room to be attached or throwing appropriate errors based on the current state.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
Sources/AblyChat/RoomLifecycleManager.swift (1)
511-513: 🛠️ Refactor suggestionConsider safer approach to error handling.
The use of
try!forces unwrapping of potential errors, which could lead to crashes if an unexpected error occurs.Consider refactoring to use a safer error handling approach:
- // swiftlint:disable:next force_try - try! await performAnOperation(forcingOperationID: forcedOperationID) { operationID in - await bodyOfReleaseOperation(operationID: operationID) - } + do { + try await performAnOperation(forcingOperationID: forcedOperationID) { operationID in + await bodyOfReleaseOperation(operationID: operationID) + } + } catch { + logger.log(message: "Error in performReleaseOperation: \(error)", level: .error) + // Since this is expected to be non-throwing, we'll handle errors internally + }
♻️ Duplicate comments (2)
Sources/AblyChat/Room.swift (1)
123-129:⚠️ Potential issueNonisolated
channelexposes a non-thread-safe Ably objectReturning the raw
RealtimeChannelProtocolfrom anonisolatedproperty defeats Swift's actor isolation and lets concurrent tasks touch the underlyingARTRealtimeChannelwithout synchronization, leading to data races.Please choose one of the following:
• Wrap all channel operations in a façade that hops to
@MainActor(or the actor instance) before forwarding calls.
• AnnotateRealtimeChannelProtocol(or thechannelproperty) with@unchecked Sendableand document that callers must confine usage to the main actor.Sources/AblyChat/RoomLifecycleManager.swift (1)
566-568: Address Task Cancellation in Sleep OperationThe code uses
try! await clock.sleep(timeInterval: waitDuration)without handling the case when theTaskis cancelled. As verified, issue #29 remains open and highlights the need to decide how our SDK should interact with Swift's Task cancellation—especially what to do whenTask.sleep(…)fails with a cancellation error.
- Current Concern: Using
try!will trap if the sleep operation is cancelled.- Actionable Suggestion: Update this block to safely handle cancellation (e.g., using proper error handling or a cancellation check) or, at a minimum, document the provisional limitations until a design decision is made.
🧹 Nitpick comments (1)
Sources/AblyChat/RoomLifecycleManager.swift (1)
209-210: Consider adding integration test for race condition prevention.The comment explains a potential race condition with ably-cocoa callbacks and how it's addressed by requiring dispatch to the main queue.
Consider adding an integration test that specifically verifies this behavior to ensure it continues to work correctly as the codebase evolves.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (45)
.github/workflows/check.yaml(1 hunks)Example/AblyChatExample/ContentView.swift(2 hunks)Example/AblyChatExample/Mocks/MockClients.swift(2 hunks)Package.swift(0 hunks)README.md(1 hunks)Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift(4 hunks)Sources/AblyChat/ChatAPI.swift(5 hunks)Sources/AblyChat/ChatClient.swift(1 hunks)Sources/AblyChat/DefaultMessages.swift(10 hunks)Sources/AblyChat/DefaultOccupancy.swift(3 hunks)Sources/AblyChat/DefaultPresence.swift(14 hunks)Sources/AblyChat/DefaultRoomLifecycleContributor.swift(0 hunks)Sources/AblyChat/DefaultRoomReactions.swift(4 hunks)Sources/AblyChat/DefaultTyping.swift(8 hunks)Sources/AblyChat/DiscontinuityEvent.swift(1 hunks)Sources/AblyChat/EmitsDiscontinuities.swift(0 hunks)Sources/AblyChat/Errors.swift(6 hunks)Sources/AblyChat/Messages.swift(1 hunks)Sources/AblyChat/Occupancy.swift(1 hunks)Sources/AblyChat/Presence.swift(3 hunks)Sources/AblyChat/Room.swift(6 hunks)Sources/AblyChat/RoomFeature.swift(1 hunks)Sources/AblyChat/RoomLifecycleManager.swift(12 hunks)Sources/AblyChat/RoomOptions.swift(2 hunks)Sources/AblyChat/RoomReactionDTO.swift(3 hunks)Sources/AblyChat/RoomReactions.swift(1 hunks)Sources/AblyChat/RoomStatus.swift(3 hunks)Sources/AblyChat/Rooms.swift(3 hunks)Sources/AblyChat/Subscription.swift(1 hunks)Sources/AblyChat/Typing.swift(1 hunks)Tests/AblyChatTests/DefaultMessagesTests.swift(5 hunks)Tests/AblyChatTests/DefaultPresenceTests.swift(1 hunks)Tests/AblyChatTests/DefaultRoomOccupancyTests.swift(2 hunks)Tests/AblyChatTests/DefaultRoomReactionsTests.swift(2 hunks)Tests/AblyChatTests/DefaultRoomTests.swift(8 hunks)Tests/AblyChatTests/DefaultRoomsTests.swift(13 hunks)Tests/AblyChatTests/IntegrationTests.swift(3 hunks)Tests/AblyChatTests/Mocks/MockFeatureChannel.swift(0 hunks)Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift(2 hunks)Tests/AblyChatTests/Mocks/MockRoom.swift(1 hunks)Tests/AblyChatTests/Mocks/MockRoomLifecycleContributor.swift(0 hunks)Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift(2 hunks)Tests/AblyChatTests/Mocks/MockRoomLifecycleManagerFactory.swift(1 hunks)Tests/AblyChatTests/RoomOptionsTests.swift(1 hunks)Tests/AblyChatTests/RoomReactionDTOTests.swift(2 hunks)
💤 Files with no reviewable changes (5)
- Package.swift
- Sources/AblyChat/DefaultRoomLifecycleContributor.swift
- Tests/AblyChatTests/Mocks/MockRoomLifecycleContributor.swift
- Tests/AblyChatTests/Mocks/MockFeatureChannel.swift
- Sources/AblyChat/EmitsDiscontinuities.swift
✅ Files skipped from review due to trivial changes (2)
- README.md
- Sources/AblyChat/Typing.swift
🚧 Files skipped from review as they are similar to previous changes (33)
- Tests/AblyChatTests/RoomReactionDTOTests.swift
- Tests/AblyChatTests/IntegrationTests.swift
- Tests/AblyChatTests/DefaultPresenceTests.swift
- Sources/AblyChat/Messages.swift
- Example/AblyChatExample/ContentView.swift
- Sources/AblyChat/ChatClient.swift
- Sources/AblyChat/ChatAPI.swift
- Sources/AblyChat/DiscontinuityEvent.swift
- Tests/AblyChatTests/RoomOptionsTests.swift
- Sources/AblyChat/Subscription.swift
- .github/workflows/check.yaml
- Sources/AblyChat/Occupancy.swift
- Sources/AblyChat/RoomReactions.swift
- Tests/AblyChatTests/DefaultMessagesTests.swift
- Tests/AblyChatTests/DefaultRoomReactionsTests.swift
- Tests/AblyChatTests/Mocks/MockRoom.swift
- Sources/AblyChat/Presence.swift
- Tests/AblyChatTests/DefaultRoomOccupancyTests.swift
- Sources/AblyChat/RoomReactionDTO.swift
- Sources/AblyChat/RoomStatus.swift
- Tests/AblyChatTests/Mocks/MockRoomLifecycleManagerFactory.swift
- Sources/AblyChat/Rooms.swift
- Tests/AblyChatTests/DefaultRoomsTests.swift
- Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift
- Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift
- Sources/AblyChat/RoomOptions.swift
- Sources/AblyChat/DefaultRoomReactions.swift
- Sources/AblyChat/DefaultTyping.swift
- Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift
- Example/AblyChatExample/Mocks/MockClients.swift
- Sources/AblyChat/DefaultMessages.swift
- Sources/AblyChat/DefaultOccupancy.swift
- Sources/AblyChat/DefaultPresence.swift
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Example app, iOS (Xcode 16.3)
- GitHub Check: Xcode, tvOS (Xcode 16.3)
- GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16.3) - GitHub Check: Xcode, iOS (Xcode 16.3)
- GitHub Check: Xcode, macOS (Xcode 16.3)
- GitHub Check: Xcode, tvOS (Xcode 16.2)
- GitHub Check: Xcode, iOS (Xcode 16.2)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16.2) - GitHub Check: Xcode, macOS (Xcode 16.2)
🔇 Additional comments (39)
Sources/AblyChat/RoomFeature.swift (1)
3-10: Good job simplifying the RoomFeature enumThe RoomFeature enum has been appropriately simplified to just the feature case declarations, which aligns perfectly with the single channel architecture refactoring. I like the removal of the channel naming utilities and other unnecessary complexity.
Sources/AblyChat/Errors.swift (8)
39-42: Appropriate addition of the roomDiscontinuity error codeAdding this new error code for discontinuity events aligns well with the architectural change to have discontinuity events at the room level rather than per feature. This helps simplify the error handling model.
51-51: Good addition to CaseThatImpliesFixedStatusCodeAdding roomDiscontinuity to the list of error cases with fixed status codes is consistent with the error handling pattern in the codebase.
65-66: Consistent implementation of toNumericErrorCode mappingThe mapping from CaseThatImpliesFixedStatusCode.roomDiscontinuity to ErrorCode.roomDiscontinuity maintains the pattern used for other error codes.
80-81: Appropriate status code for discontinuity errorsUsing status code 500 for discontinuity events is appropriate as these represent server-side or system errors rather than client errors.
141-141: Good error case design with optional causeAdding the roomDiscontinuity case with an optional error cause allows for proper propagation of underlying errors from the Ably library.
164-165: Consistent error mapping implementationThe mapping from ChatError.roomDiscontinuity to ErrorCodeAndStatusCode.fixedStatusCode(.roomDiscontinuity) follows the established pattern.
210-211: Clear error message for room discontinuityThe error message is concise and clearly communicates that a discontinuity has occurred.
220-221: Proper cause propagation for discontinuity errorsIncluding the underlying cause for discontinuity errors ensures that detailed error information is preserved.
Tests/AblyChatTests/DefaultRoomTests.swift (8)
9-22: Good test for channel name in the new architectureThis test effectively verifies that the internal channel name follows the expected format ("basketball::$chat") in the new single-channel architecture.
43-60: Appropriate updates to channel fetching testThe renamed test
fetchesChannelOnce()correctly verifies that only one channel is fetched with the appropriate options, including merged parameters for presence and occupancy features.
62-89: Great parameterized test for occupancy event flagThis test thoroughly verifies that enabling or disabling occupancy events correctly sets the appropriate channel parameters. The parameterized approach keeps the test concise while covering both cases.
91-118: Excellent test for presence events configurationThis parameterized test thoroughly verifies that the presence enableEvents flag correctly affects the channel modes. The comments explaining the expected behavior are helpful.
123-134: Good simplification of lifecycle manager testThe updated test correctly verifies that the single channel is passed to the lifecycle manager, replacing the more complex test that verified contributor order and feature sets.
246-246: Status tests updated correctly for new error parameterThe test updates reflect the changes to RoomStatus initializers that now include error information for all states, which aligns with the PR objective.
281-306: Comprehensive test for discontinuity eventsThis new test effectively verifies that discontinuity events from the lifecycle manager are properly propagated through the room's onDiscontinuity() method. The test checks both subscription setup and event delivery.
308-309: Clear documentation of non-applicable spec pointsThe comments clearly explain why certain spec points are not applicable due to the use of AsyncSequence instead of explicit listener/unsubscribe APIs.
Sources/AblyChat/Room.swift (9)
108-122: Good addition of discontinuity event methodsAdding these methods to the Room protocol properly enables clients to subscribe to discontinuity events, which aligns with the architectural changes to handle discontinuities at the room level.
136-138: Good default implementation for onDiscontinuity()The default implementation for onDiscontinuity() follows the same pattern as onStatusChange(), providing a convenient way to subscribe with unbounded buffering.
176-176: Simplified factory return typeRemoving the generic parameter and returning a concrete DefaultRoom simplifies the code and aligns with the removal of other generic parameters in the refactoring.
193-198: Good change from optional to non-optional propertiesMaking these feature properties non-optional simplifies the client code by eliminating the need for unwrapping and reflects that in the new architecture these features are always available.
205-208: Good separation of internal and exposed channelThe comment clearly explains that this property exists to satisfy the Room interface and shouldn't be used within the class. This helps maintain a clean separation between internal and external APIs.
210-214: Useful test-only exposure of internal channelExposing the internal channel for testing purposes with the DEBUG flag is a good practice to maintain test coverage without compromising the production API.
225-298: Well-implemented channel creation and feature initializationThe channel creation logic has been effectively centralized in the createChannel method, with clear comments referencing specification points. The feature initialization code properly uses the single internal channel and includes the necessary options.
316-321: Clean release implementationThe release method has been simplified to release only the single internal channel, which is consistent with the new architecture.
333-337: Good implementation of discontinuity handlingThe implementation of onDiscontinuity properly delegates to the lifecycle manager, which centralizes discontinuity handling as intended in the new architecture.
Sources/AblyChat/RoomLifecycleManager.swift (13)
10-21: Well-documented protocol enhancement.The addition of
waitToBeAbleToPerformPresenceOperationswith thorough documentation clearly explains the conditions for performing presence operations in different room states. This aligns with the single-channel architecture refactor.
23-23: Good addition of discontinuity event API.The new
onDiscontinuitymethod properly exposes channel discontinuity events at the room lifecycle level, which is a key part of the single-channel architecture.
30-34: Clean factory interface update.The factory method signature has been appropriately updated to create a manager with a single channel instead of multiple contributors, aligning with the architectural refactor.
51-78: Excellent state mapping implementation.The
RoomStatusextension provides a clean, centralized mapping from Ably channel states to the SDK's room status, ensuring consistency throughout the codebase.
92-102: Clear tracking of room attachment state.The implementation properly tracks whether the room has ever been attached and whether it was explicitly detached, which is crucial for the discontinuity detection logic.
161-164: Well-implemented channel event listener.The channel state event listener is correctly set up to capture state changes from the Ably channel and propagate them to the room lifecycle manager.
167-173: Properly handled event listener cleanup.The
deinitmethod correctly removes the channel state event listener asynchronously on the main actor, preventing potential memory leaks or crashes.
194-219: Robust channel state handling with discontinuity detection.The implementation correctly processes channel state events and detects discontinuities based on the channel's resumed flag, attachment history, and explicit detachment state.
221-227: Clean discontinuity event subscription.The implementation of the discontinuity event subscription mechanism is clean and follows the same pattern as other subscription methods in the codebase.
409-426: Simplified attach operation.The attach operation has been successfully simplified to work directly with a single channel while properly maintaining state flags and handling errors.
475-491: Well-implemented detach operation.The detach operation correctly handles the transition to detached state, manages the explicit detachment flag, and handles errors appropriately.
540-575: Robust release operation with retry logic.The release operation implementation includes proper retry logic for detaching the channel before releasing, with appropriate logging and status updates.
587-612: Well-implemented presence operation check.The implementation correctly handles the different room states and waits for status changes when needed, throwing appropriate errors based on the current status.
We'll be getting rid of the *AttachmentFailed error codes as part of the upcoming single-channel migration. The spec does not say to use the MessagesAttachmentFailed error code as part of the messages feature, so remove its use here and replace it with something generic. (I don't know what _is_ the right thing to use here since I'm not familiar with this functionality; from a glance of the spec it doesn't seem like the code we've written here comes from any behaviour described in the spec.)
We added the same thing to the SDK in 3d63cec.
Was fixed in spec commit 47b82da.
I want us to start referring to a generic ARTChannelStateChange object as a "channel event", not a "channel state change", since, despite its unfortunate naming, such an object actually refers either a state change or an UPDATE event. There are still plenty of places in the codebase where the naming needs to be updated, which I'll address during the upcoming single-channel work to avoid churn.
We'll revert this in #281. [1] ably/specification#282
Resolves #242. The API changes are based on the JS API changes from branch integration/single-channel-integration at 0924cd5. The behavioural changes are based on [1] at 2f88b1b. This commit is quite large, and combines API and behavioural changes, because the new specification points are written with reference to the new API and so it was hard to disentangle the two. Key changes to public API: - Updated room options API: - there's no concept of a feature being turned "on" any more, and correspondingly no error when you try and use a feature that's not turned on - removed `allFeaturesEnabled`, since the equivalent has been removed from JS - you can now fetch a room without passing a room options; this uses the default options - Removed feature-specific error codes - Discontinuities now always have an error - Extended the set of RoomStatus that can have an error (this does not have an equivalent in the JS API; it's a result of the room lifecycle manager statuses that mean we're now more driven by the errors emitted by ably-cocoa; we still need to revisit this in #12) Key changes to interactions with Realtime: - Switched to v3 of the REST API - Use a single channel for all room features; this simplifies the room lifecycle manager and the DefaultRoom class - Room reactions now use ephemeral messages - Resolves #133 (setting of presence-related channel modes). Internal changes: - Removed FeatureChannel and RoomLifecycleContributor - Switch to using callbacks for ably-cocoa state changes (i.e. end the experiment of using AsyncSequence in the ably-cocoa wrapper) and enforce that ably-cocoa deliver its state changes on the main thread; together with fc83fc1 this allows us to no longer have to work around the fact that the spec's method for detecting a discontinuity assumes a single-threaded environment, hence resolving #239. [1] ably/specification#282
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift (1)
260-262:⚠️ Potential issueMissing toAblyCocoaCallback wrapper in subscribe method
Unlike other callback methods that use
toAblyCocoaCallback, thissubscribemethod passes the callback directly to the underlying implementation without wrapping it to enforce main thread execution.internal func subscribe(_ name: String, callback: @escaping @MainActor (ARTMessage) -> Void) -> ARTEventListener? { - underlying.subscribe(name, callback: callback) + underlying.subscribe(name, callback: toAblyCocoaCallback(callback)) }
♻️ Duplicate comments (3)
Sources/AblyChat/Room.swift (1)
205-208: 🛠️ Refactor suggestionImplementation note acknowledges potential issue
The comment indicates awareness that the
channelproperty is only exposed to satisfy the interface. This reinforces the concern about thread safety in the exposed property.Consider implementing a safer wrapper for the exposed channel:
-// Note: This property only exists to satisfy the `Room` interface. Do not use this property inside this class; use `internalChannel`. -internal nonisolated var channel: any RealtimeChannelProtocol { - internalChannel.underlying -} +/// Thread-safe wrapper for the underlying Ably channel +internal nonisolated var channel: any RealtimeChannelProtocol { + let underlyingChannel = internalChannel.underlying + // Return a thread-safe proxy that redirects all calls to the main actor + return ThreadSafeChannelProxy(wrapping: underlyingChannel) +}This would require implementing a
ThreadSafeChannelProxyclass that implementsRealtimeChannelProtocoland redirects all method calls to the main actor.Sources/AblyChat/RoomLifecycleManager.swift (2)
543-572:⚠️ Potential issuePersistent task cancellation issue in release operation
The release operation still contains an unresolved TODO for handling task cancellation during sleep operations (lines 566-568). This was flagged in a previous review and is still using
try!which could cause crashes if a task is cancelled during the sleep operation.This needs to be addressed as noted in issue #29. Consider implementing proper cancellation handling:
- // TODO: Make this not trap in the case where the Task is cancelled (as part of the broader https://github.com/ably-labs/ably-chat-swift/issues/29 for handling task cancellation) - // swiftlint:disable:next force_try - try! await clock.sleep(timeInterval: waitDuration) + do { + try await clock.sleep(timeInterval: waitDuration) + } catch is CancellationError { + // Clean up and exit the release operation + logger.log(message: "Release operation was cancelled during retry wait", level: .info) + break + }
599-605:⚠️ Potential issueTask cancellation handling needed in waitToBeAbleToPerformPresenceOperations
The method still contains an unresolved TODO about handling the case when
nextRoomStatusChangeis nil due to task cancellation (lines 601-602).Consider implementing proper cancellation handling:
- // TODO: decide what to do if nextRoomStatusChange is nil; I believe that this will happen if the current Task is cancelled. For now, will just treat it as an invalid status change. Handle it properly in https://github.com/ably-labs/ably-chat-swift/issues/29 - if nextRoomStatusChange?.current != .attached(error: nil) { + guard let nextChange = nextRoomStatusChange else { + // Task was likely cancelled + try Task.checkCancellation() + // If we get here, the sequence terminated without a cancellation + throw ARTErrorInfo(chatError: .roomTransitionedToInvalidStateForPresenceOperation(cause: nil)).toInternalError() + } + + if nextChange.current != .attached(error: nil) {
🧹 Nitpick comments (1)
Sources/AblyChat/RoomFeature.swift (1)
3-10: Good simplification of the RoomFeature enumThe simplification of the
RoomFeatureenum aligns with the overall architectural change to consolidate features onto a single channel. However, consider adding documentation comments to each case to explain their purpose and usage context.internal enum RoomFeature { - case messages - case presence - case typing - case reactions - case occupancy + /// Messages sent in the room + case messages + /// Presence information about users in the room + case presence + /// Typing indicators from users in the room + case typing + /// Reactions to messages in the room + case reactions + /// Room occupancy metrics + case occupancy }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (48)
.github/workflows/check.yaml(1 hunks)Example/AblyChatExample/ContentView.swift(2 hunks)Example/AblyChatExample/Mocks/MockClients.swift(2 hunks)Package.swift(0 hunks)README.md(1 hunks)Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift(4 hunks)Sources/AblyChat/ChatAPI.swift(5 hunks)Sources/AblyChat/ChatClient.swift(1 hunks)Sources/AblyChat/DefaultMessages.swift(10 hunks)Sources/AblyChat/DefaultOccupancy.swift(3 hunks)Sources/AblyChat/DefaultPresence.swift(14 hunks)Sources/AblyChat/DefaultRoomLifecycleContributor.swift(0 hunks)Sources/AblyChat/DefaultRoomReactions.swift(4 hunks)Sources/AblyChat/DefaultTyping.swift(8 hunks)Sources/AblyChat/DiscontinuityEvent.swift(1 hunks)Sources/AblyChat/EmitsDiscontinuities.swift(0 hunks)Sources/AblyChat/Errors.swift(6 hunks)Sources/AblyChat/Messages.swift(1 hunks)Sources/AblyChat/Occupancy.swift(1 hunks)Sources/AblyChat/Presence.swift(3 hunks)Sources/AblyChat/Room.swift(6 hunks)Sources/AblyChat/RoomFeature.swift(1 hunks)Sources/AblyChat/RoomLifecycleManager.swift(12 hunks)Sources/AblyChat/RoomOptions.swift(2 hunks)Sources/AblyChat/RoomReactionDTO.swift(3 hunks)Sources/AblyChat/RoomReactions.swift(1 hunks)Sources/AblyChat/RoomStatus.swift(3 hunks)Sources/AblyChat/Rooms.swift(3 hunks)Sources/AblyChat/Subscription.swift(1 hunks)Sources/AblyChat/Typing.swift(1 hunks)Tests/AblyChatTests/ChatAPITests.swift(5 hunks)Tests/AblyChatTests/DefaultMessagesTests.swift(5 hunks)Tests/AblyChatTests/DefaultPresenceTests.swift(1 hunks)Tests/AblyChatTests/DefaultRoomOccupancyTests.swift(2 hunks)Tests/AblyChatTests/DefaultRoomReactionsTests.swift(2 hunks)Tests/AblyChatTests/DefaultRoomTests.swift(8 hunks)Tests/AblyChatTests/DefaultRoomsTests.swift(13 hunks)Tests/AblyChatTests/Helpers/RoomLifecycle+Error.swift(0 hunks)Tests/AblyChatTests/IntegrationTests.swift(3 hunks)Tests/AblyChatTests/Mocks/MockFeatureChannel.swift(0 hunks)Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift(3 hunks)Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift(2 hunks)Tests/AblyChatTests/Mocks/MockRoom.swift(1 hunks)Tests/AblyChatTests/Mocks/MockRoomLifecycleContributor.swift(0 hunks)Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift(2 hunks)Tests/AblyChatTests/Mocks/MockRoomLifecycleManagerFactory.swift(1 hunks)Tests/AblyChatTests/RoomOptionsTests.swift(1 hunks)Tests/AblyChatTests/RoomReactionDTOTests.swift(2 hunks)
💤 Files with no reviewable changes (6)
- Package.swift
- Sources/AblyChat/EmitsDiscontinuities.swift
- Tests/AblyChatTests/Mocks/MockFeatureChannel.swift
- Sources/AblyChat/DefaultRoomLifecycleContributor.swift
- Tests/AblyChatTests/Mocks/MockRoomLifecycleContributor.swift
- Tests/AblyChatTests/Helpers/RoomLifecycle+Error.swift
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (36)
- Tests/AblyChatTests/DefaultPresenceTests.swift
- .github/workflows/check.yaml
- Tests/AblyChatTests/RoomReactionDTOTests.swift
- Tests/AblyChatTests/ChatAPITests.swift
- Sources/AblyChat/RoomReactionDTO.swift
- Tests/AblyChatTests/RoomOptionsTests.swift
- Sources/AblyChat/Occupancy.swift
- Tests/AblyChatTests/Mocks/MockRoom.swift
- Sources/AblyChat/ChatAPI.swift
- Sources/AblyChat/Messages.swift
- Sources/AblyChat/DiscontinuityEvent.swift
- Tests/AblyChatTests/IntegrationTests.swift
- Sources/AblyChat/ChatClient.swift
- Example/AblyChatExample/ContentView.swift
- Sources/AblyChat/Typing.swift
- Sources/AblyChat/RoomReactions.swift
- Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift
- Tests/AblyChatTests/DefaultMessagesTests.swift
- Sources/AblyChat/Presence.swift
- Sources/AblyChat/Rooms.swift
- Sources/AblyChat/Subscription.swift
- Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift
- Tests/AblyChatTests/Mocks/MockRoomLifecycleManagerFactory.swift
- Sources/AblyChat/DefaultRoomReactions.swift
- Tests/AblyChatTests/DefaultRoomOccupancyTests.swift
- Sources/AblyChat/RoomStatus.swift
- Sources/AblyChat/Errors.swift
- Tests/AblyChatTests/DefaultRoomReactionsTests.swift
- Tests/AblyChatTests/DefaultRoomsTests.swift
- Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift
- Example/AblyChatExample/Mocks/MockClients.swift
- Sources/AblyChat/DefaultOccupancy.swift
- Sources/AblyChat/DefaultMessages.swift
- Sources/AblyChat/RoomOptions.swift
- Sources/AblyChat/DefaultTyping.swift
- Sources/AblyChat/DefaultPresence.swift
🧰 Additional context used
🧬 Code Graph Analysis (1)
Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift (3)
Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (3)
on(156-158)on(160-164)subscribe(135-142)Tests/AblyChatTests/Mocks/MockConnection.swift (1)
on(14-16)Tests/AblyChatTests/Mocks/MockRealtimePresence.swift (2)
subscribe(5-7)subscribe(9-11)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16.2) - GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16.2) - GitHub Check: Xcode, tvOS (Xcode 16.3)
- GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16.1) - GitHub Check: Example app, tvOS (Xcode 16.3)
- GitHub Check: Xcode, iOS (Xcode 16.3)
- GitHub Check: Example app, iOS (Xcode 16.3)
- GitHub Check: Xcode, macOS (Xcode 16.3)
- GitHub Check: Xcode, tvOS (Xcode 16.2)
- GitHub Check: Example app, tvOS (Xcode 16.2)
- GitHub Check: Xcode, iOS (Xcode 16.2)
- GitHub Check: Example app, iOS (Xcode 16.2)
- GitHub Check: Xcode, macOS (Xcode 16.2)
- GitHub Check: SPM (Xcode 16.3)
- GitHub Check: Xcode, tvOS (Xcode 16.1)
- GitHub Check: Example app, macOS (Xcode 16.2)
- GitHub Check: Xcode, iOS (Xcode 16.1)
- GitHub Check: Example app, tvOS (Xcode 16.1)
- GitHub Check: Xcode, macOS (Xcode 16.1)
- GitHub Check: Example app, iOS (Xcode 16.1)
🔇 Additional comments (17)
Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift (1)
92-118: Well-documented main thread callback handlingThe implementation of
toAblyCocoaCallbackprovides a necessary workaround for Swift's concurrency checks while ensuring callbacks execute on the main thread. The thorough documentation is excellent, particularly the warning about mutability concerns.Tests/AblyChatTests/DefaultRoomTests.swift (3)
9-22: Good test for the single channel architectureThis new test correctly verifies that the room uses the expected channel name, which is important for validating the architectural change to a single channel per room.
62-118: Well-structured parametrized tests for feature flagsThese parameterized tests thoroughly verify that enabling or disabling features correctly sets the appropriate channel parameters and modes. This is excellent test coverage for the configuration logic.
281-306: Good test coverage for discontinuity eventsThe new test for discontinuity events properly validates that events emitted by the lifecycle manager propagate correctly through the room's API. The test uses clear, consistent patterns and assertions.
Sources/AblyChat/Room.swift (2)
277-298: Well-structured channel creation methodThe static
createChannelmethod provides a clear, organized approach to configuring channel options based on room options. The comments that reference specification points are particularly helpful for understanding the implementation decisions.
333-337: Good implementation of discontinuity event handlingThe implementation of
onDiscontinuityproperly delegates to the lifecycle manager, maintaining a clean separation of concerns.Sources/AblyChat/RoomLifecycleManager.swift (11)
10-24: API improvement with new discontinuity event handlingThe addition of the
onDiscontinuitymethod (line 23) aligns with the architectural changes described in the PR objectives, providing a clean way to expose channel discontinuity events at the room level.
31-32: Factory signature correctly simplifiedThe factory method signature now accepts a single channel instead of multiple contributors, correctly implementing the architectural change to use a single channel per room.
51-78: Well-structured mapping from channel state to room statusThe new
RoomStatusextension creates a clear mapping between the Ably channel states and the SDK's room statuses, handling all possible states including error conditions. The use offatalErrorfor unexpected cases ensures correctness during development.
85-107: Simplified state management with single channelThe class properties have been correctly simplified to track status for a single channel rather than multiple contributors. The addition of discontinuity subscriptions (line 106) supports the new discontinuity event propagation mechanism.
162-164: Proper event listener implementationThe channel state event listener is correctly implemented using a weak self reference to avoid retain cycles. The handler delegates to a method for processing the event, maintaining good separation of concerns.
168-173: Avoid potential memory leak in deinitThe deinit method correctly removes the channel state event listener. The tasks are dispatched to the main actor to ensure thread safety when removing the listener. The commented explanation acknowledges the workaround nature of this approach.
195-219: Properly implemented discontinuity detectionThe
didReceiveChannelStateEventmethod correctly implements the specification requirements for detecting and propagating discontinuities (lines 210-215). The implementation includes appropriate logging and only emits a discontinuity event when the conditions in line 210 are met.
409-427: Simplified attach operationThe attach operation has been correctly simplified to work with a single channel. It properly tracks the state changes, handles errors, and updates the
hasAttachedOnceandisExplicitlyDetachedflags.
475-492: Simplified detach operationThe detach operation implementation is clear and follows the correct state transitions. It updates the
isExplicitlyDetachedflag (line 490) to correctly track when a detach was explicitly requested by the user.
195-195: Great implementation note regarding threading and discontinuity detectionThe comment on lines 209-210 provides excellent context about the requirement that the Ably client be configured to use the main queue for callbacks. This is important for maintaining the correct behavior of discontinuity detection.
Also applies to: 209-210
211-215: Proper error handling for discontinuitiesThe discontinuity event generation uses the new
roomDiscontinuityerror code, which aligns with the PR objective of removing feature-specific error codes in favor of a unified approach.
2 participants
Note: This is based on top of #250; its failing test is because of a failing test in that PR.
Resolves #242.
The API changes are based on the JS API changes from branch
integration/single-channel-integrationat0924cd5. The behavioural changes are based on ably/specification#282 at27ddbb3.Key changes to public API:
allFeaturesEnabled, since the equivalent has been removed from JSRoomStatusthat can have an error (this does not have an equivalent in the JS API; it's a result of the room lifecycle manager statuses that mean we're now more driven by the errors emitted by ably-cocoa; we still need to revisit this in Consider how to avoid the need for an unwrap oferrorproperties #12)Key changes to interactions with Realtime:
DefaultRoomclassInternal changes:
FeatureChannelandRoomLifecycleContributorAsyncSequencein the ably-cocoa wrapper) and enforce that ably-cocoa deliver its state changes on the main thread; together with fc83fc1 this allows us to no longer have to work around the fact that the spec's method for detecting a discontinuity assumes a single-threaded environment, hence resolving [ECO-5251] Presence tests (single channel) #239.Summary by CodeRabbit
New Features
Improvements
enableEventsflags.Bug Fixes
ephemeralflag.Refactor
Tests
Chores